-
Notifications
You must be signed in to change notification settings - Fork 14k
compiletest: Don't apply "emscripten" directives to wasm32-unknown-unknown
#148969
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@bors try jobs=test-various,dist-various-1 |
This comment has been minimized.
This comment has been minimized.
Don't apply "emscripten" directives to `wasm32-unknown-unknown` try-job: test-various try-job: dist-various-1
wasm32-unknown-unknownwasm32-unknown-unknown
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Some changes occurred in src/tools/compiletest cc @jieyouxu |
|
rustbot has assigned @Mark-Simulacrum. Use |
|
Is w32-u-u even tested? I think it was replaced by w32-wasip1 in test-various? |
The only reference to So I suspect the w32uu target is not actually tested anywhere in our CI. Which I guess makes sense for a tier 2 target. |
|
Does #146881 (comment) mean that w32uu is already broken when trying to run the full test suite? Because if it's already broken (and therefore untested), then that makes me even more keen to remove a compiletest special case that doesn't seem to be helping anybody. |
|
I agree that the special case isn't helping. I think in one case the target is broken, in the other it's the test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree, this implication has always striken me as being... weird. If a test has to be ignored for wasm32-u-u and emscripten, that should use two directives, or use something like target-arch based ignores.
|
r? me @bors r+ rollup |
|
The rustc-dev-guide subtree was changed. If this PR only touches the dev guide consider submitting a PR directly to rust-lang/rustc-dev-guide otherwise thank you for updating the dev guide with your changes. |
|
Realised that this PR obsoleted some text in the dev guide, so I pushed a trivial update to remove it. @bors r=jieyouxu |
| - Environment (fourth word of the target triple): `gnu`, `msvc`, `musl` | ||
| - WASM: `wasm32-bare` matches `wasm32-unknown-unknown`. `emscripten` also | ||
| matches that target as well as the emscripten targets. | ||
| - WASM: `wasm32-bare` matches `wasm32-unknown-unknown`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This alias also dates back to the introduction of wasm32-unknown-unknown, and doesn't seem to serve any useful purpose compared to just writing the actual target name.
I'll leave its removal to a follow-up PR.
Rollup merge of #148969 - Zalathar:wasm32-uwu, r=jieyouxu compiletest: Don't apply "emscripten" directives to `wasm32-unknown-unknown` This special case dates all the way back to the original introduction of the `wasm32-unknown-unknown` target, in #45905. Either it isn't needed, in which case we should remove it, or it *is* needed, in which case we should fix the directives in any affected tests. Note that while the intent of this code was presumably to make `//@ ignore-emscripten` also ignore tests on w32-u-u, it has the unfortunate side-effect of also causing `//@ only-emscripten` tests to *run* on w32-u-u, which is potentially very confusing.
This special case dates all the way back to the original introduction of the
wasm32-unknown-unknowntarget, in #45905.Either it isn't needed, in which case we should remove it, or it is needed, in which case we should fix the directives in any affected tests.
Note that while the intent of this code was presumably to make
//@ ignore-emscriptenalso ignore tests on w32-u-u, it has the unfortunate side-effect of also causing//@ only-emscriptentests to run on w32-u-u, which is potentially very confusing.